Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

json-ld response formatting #399

Merged
merged 3 commits into from
Feb 28, 2023
Merged

Conversation

dpetran
Copy link
Contributor

@dpetran dpetran commented Feb 24, 2023

This allows us to use the context to inform how we are materializing the flakes into json-ld.

By specifying a predicate in the context with the {"@container" "@set"} annotation we can force that predicate's object to be wrapped in a vector, even if there is only one.

This is standards compliant - I had tried to unsuccessfully to get the json-ld playground to exhibit this behavior during compaction, but I was just holding it wrong. I had provided the annotation in the input @context, not the "compacting" @context.

@dpetran dpetran requested a review from a team February 24, 2023 15:05
@dpetran dpetran marked this pull request as ready for review February 24, 2023 15:05
In json-ld, everything is multicardinality under the covers. As a convenience, we take
predicates that have only one object and unwrap them from their vectors. If a user wants
to retain the vector that the object sits in regardless of the number of objects, they
can use the context to specify {@container: @set} (or @list) for that predicate.

fixes #390
@dpetran dpetran force-pushed the feature/json-ld-response-formatting branch from 9408324 to 237476b Compare February 24, 2023 15:06
@@ -150,12 +135,18 @@
{id-key c-iri}))
(flake/o f))]
(recur r (conj acc res)))
(if (= 1 (count acc))
(cond
(#{:list :set} (-> context (get p-iri) :container))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very high-hit (performance sensitive) part of the code. I'd prefer this be written as:

 (if (and (= 1 (count acc))
          (not (#{:list :set} (-> context (get p-iri) :container)))
   (first acc) 
   acc)

Doing two map "get" operations then a set comparison every time is not cheap in the context of possibly millions of times for a single query... at least with the above we are only doing that check when necessary.

@@ -41,12 +41,12 @@
(defprotocol ValueSelector
"Format a where search solution (map of pattern matches) by extracting and
displaying relevant pattern matches."
(format-value [fmt db iri-cache compact error-ch solution]))
(format-value [fmt db iri-cache context compact error-ch solution]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're passing both the context value and the compacting function compact here. Was there an issue with adding the new 2-arity function to compact data maps by accepting both a context and the data value that needs compacting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The json-ld/compact-fn function take a context and optionally a "used-atom" to track which context entries are used. It then transforms the supplied context and closes over the result in order to do efficient compaction and returns a single-arity function that takes an iri.

I considered adding another arity to the returned compact-fn that would accept a context along with the iri, but that forces us to re-transform the context for every compaction, an operation that can be quite costly if the context is large. So I elected to do the more clumsy approach of passing both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about it, though, if we did go with the 2-arity compact-fn it would be called in multiple places with the same context, so if we added an LRU cache of size 1 in front of it that could make the code more elegant and not incur the penalty of re-processing the context for each compaction. Or just use a delay?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The atom isn't used for any of the compacting functions in this pull request; it's only used for commits.

As I mentioned on our last call, I think we should define a new function that takes a context and data needed to be compacted using that context and returns the compacted data for these usages, and we should only pass the context through these call stacks, not any compacting functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that compact is called multiple times in the same call stack, and doing the work to compact is expensive, which is why we rely on the compact-fn to do the expensive work once.

In the the fluree.db.query.exec.select namespace it looks like each of the different Selectors can call compact to format the results, and my understanding is that several Selectors may be called during one query. If that's the case, then re-creating the compact-fn for each of those calls could get expensive.

But! My understanding of how Selectors work may be wrong - if only one of them is used than we would only have one compact-fn per query.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, your understanding is correct. I think we can leave the extra contexts in for now possibly to re-think how we do the compaction and expansion later.

Copy link
Contributor

@zonotope zonotope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👔

@dpetran dpetran merged commit 5642e08 into main Feb 28, 2023
@dpetran dpetran deleted the feature/json-ld-response-formatting branch February 28, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants